-
Notifications
You must be signed in to change notification settings - Fork 12.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: disallows unicode escape sequence in JSX #48609
chore: disallows unicode escape sequence in JSX #48609
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do this check in the checker instead. It's not scalable to add a parameter to parseRightSideOfDot
(which is used everywhere) for this.
It require information from |
I checked, and that function only has a couple of call sites (but I have no doubt that it's called a lot); I think adding another required param is not too expensive here. |
I think you had a bad merge; there's a load of commits on this PR now that shouldn't be. |
134e4ba
to
1c40ed5
Compare
1c40ed5
to
63cf201
Compare
merge conflict fixed. |
rebased |
ping @RyanCavanaugh |
eba9910
to
9851e90
Compare
hello, @RyanCavanaugh can we have this merged? it has been nearly 1 year. This is the blocker of JSX specification cleanup. |
I'm not sure Ryan has had a chance to look at this, but coming back to this (a lot later), this is how I would have implemented this myself, so I think it's okay. |
@typescript-bot test this |
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at 4e1734a. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 4e1734a. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the perf test suite on this PR at 4e1734a. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at 4e1734a. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the extended test suite on this PR at 4e1734a. You can monitor the build here. |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 4e1734a. You can monitor the build here. |
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 4e1734a. You can monitor the build here. Update: The results are in! |
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
Heya @jakebailey, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@jakebailey Here they are:
CompilerComparison Report - main..48609
System
Hosts
Scenarios
TSServerComparison Report - main..48609
System
Hosts
Scenarios
StartupComparison Report - main..48609
System
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
@jakebailey Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Detailspalantir/blueprint
|
I've merged main back into this to fix the conflict. My impression from a recent discussion is that we are looking to move parse-checks out of the checker and into the parser, and I think this is an example of that. |
enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
cc @RyanCavanaugh can we merge this |
close #48596